-
-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Dynu.com #285
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for the contribution!!! Especially on new year, impressive 🎉 !
Just a few nit-picking comments
case len(p.password) == 0: | ||
return errors.ErrEmptyPassword | ||
case p.host == "*": | ||
return errors.ErrHostWildcard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wildcards are not supported then right? Are custom hosts (i.e. a.example.com
) supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use @ or * it updates all DDNS IPs for my domains on Dynu, * doesn't update subdomains of a concrete domain, so *.domain.com
updates domain.com but not subdomain.domain.com
There's an alias
(subdomain) parameter that could be used to update a concrete subdomain, to update a bunch of domains and subdomains in one call there's an optional parameter called location
which could be used if configured in Dynu, would be similar to * but you have to set your domains and subdomains to a group in dynu.
I'm adding more features to support all these parameters, I'll update the pull as soon as I finish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use @ or * it updates all DDNS IPs for my domains on Dynu, * doesn't update subdomains of a concrete domain, so *.domain.com updates domain.com but not subdomain.domain.com
I'm a bit confused here.
Correct me if I'm wrong:
@
for domain.com updates onlydomain.com
but not `sub.domain.com*
for domain.com updatesdomain.com
,domain2.com
but notsub.domain.com
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, @
does nothing really, if you don't set the hostname
or if it's not a valid hostname for your user it will update all records for your domains that don't have a group assigned.
*
doesn't acts like a wildcard, you can use it in the hostname but it will update only the domain.com
and not sub.domain.com
I finally added support for most options on Dynu, feel free to review and give any advices about code Example usage: (Comments are for reference)
|
case len(p.password) == 0: | ||
return errors.ErrEmptyPassword | ||
case p.host == "*": | ||
return errors.ErrHostWildcard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use @ or * it updates all DDNS IPs for my domains on Dynu, * doesn't update subdomains of a concrete domain, so *.domain.com updates domain.com but not subdomain.domain.com
I'm a bit confused here.
Correct me if I'm wrong:
@
for domain.com updates onlydomain.com
but not `sub.domain.com*
for domain.com updatesdomain.com
,domain2.com
but notsub.domain.com
I removed the mess with extra params and simplified the usage, adding only an extra parameter for the group.
|
Any news about these changes? I'm currently using it and it's working fine, we could merge it into master if you agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay this got lost in my emails 😢
Just a few comments and let's merge this ASAP 😉
Thanks again for your work!
Just built and am using an image of this branch for Dynu as well, everything seems to working fine. Thanks guys 😁 |
Done! |
Awesome thanks for your work and dedication @msxdan |
Added support for dynu. I could only test IPv4 as my ISP doesn't provide me an IPv6 but I added the right param to the query so it should work as good as IPv4 does.
Closes #222, #237, #263, #269